C# API review fixes#1343
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
|
||
| // Protocol v2 backward-compatibility adapters | ||
|
|
||
| public async ValueTask<ToolCallResponseV2> OnToolCallV2(string sessionId, |
There was a problem hiding this comment.
Do we need to maintain the V2 support?
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1343 · ● 946.3K
| /// </code> | ||
| /// </example> | ||
| public IDisposable On(string eventType, Action<SessionLifecycleEvent> handler) | ||
| public IDisposable OnLifecycle<T>(Action<T> handler) where T : SessionLifecycleEvent |
There was a problem hiding this comment.
Cross-SDK consistency note: The lifecycle event subscription API has changed from two overloads (On(handler) and On(eventType, handler)) to a single generic method OnLifecycle<T>(handler). The other SDKs use a different pattern:
- Node.js:
client.on(handler)andclient.on(eventType, handler) - Python:
client.on(handler)andclient.on(event_type, handler) - Go:
client.On(handler)andclient.OnEventType(eventType, handler)
The generic type approach is idiomatic .NET and has real advantages (compile-time type safety), but the method name OnLifecycle also diverges from On / on. If this design is retained, it's worth documenting that this is a .NET-specific pattern for filtering by event type, and considering whether the approach (if not the exact API shape) should influence the other SDKs.
| /// When provided, the server will route <c>exitPlanMode.request</c> callbacks to this handler. | ||
| /// </summary> | ||
| public ExitPlanModeHandler? OnExitPlanMode { get; set; } | ||
| public ExitPlanModeHandler? OnExitPlanModeRequest { get; set; } |
There was a problem hiding this comment.
Cross-SDK consistency note: The property has been renamed from OnExitPlanMode to OnExitPlanModeRequest, but the equivalent property in the other SDKs still uses the original name (without the Request suffix):
- Node.js:
SessionConfig.onExitPlanMode - Python:
create_session(..., on_exit_plan_mode=...) - Go:
SessionConfig.OnExitPlanMode
If this rename is intentional as an improvement to the API, the same change should be considered for the other SDKs. If the goal is cross-SDK consistency, the other SDKs should eventually align with this new naming (or this property should revert to OnExitPlanMode).
| /// This is used only when <see cref="CopilotClientOptions.SessionFs"/> is configured. | ||
| /// </summary> | ||
| public Func<CopilotSession, SessionFsProvider>? CreateSessionFsHandler { get; set; } | ||
| public Func<CopilotSession, SessionFsProvider>? CreateSessionFsProvider { get; set; } |
There was a problem hiding this comment.
Cross-SDK consistency note: This has been renamed from CreateSessionFsHandler to CreateSessionFsProvider, but all other SDKs use the ...Handler naming:
- Node.js:
SessionConfig.createSessionFsHandler - Python:
create_session(..., create_session_fs_handler=...) - Go:
SessionConfig.CreateSessionFsHandler
Interestingly, SessionFsProvider is already the name of the returned type in Node.js and Python too, so CreateSessionFsProvider could be seen as more descriptive. If this name is preferred, it may be worth aligning the other SDKs.
| /// Default: false (resume event is emitted). | ||
| /// </summary> | ||
| public bool DisableResume { get; set; } | ||
| public bool SuppressResumeEvent { get; set; } |
There was a problem hiding this comment.
Cross-SDK consistency note: This has been renamed from DisableResume to SuppressResumeEvent, but:
- Node.js:
ResumeSessionConfig.disableResume - Go:
ResumeSessionConfig.DisableResume - Python: Planned as
disable_resume(TODO comment in source)
The new name SuppressResumeEvent is arguably more descriptive, but it diverges from the other SDK naming. If this naming is preferred, the other SDKs should be updated to match.
| /// When provided, the server will route <c>autoModeSwitch.request</c> callbacks to this handler. | ||
| /// </summary> | ||
| public AutoModeSwitchHandler? OnAutoModeSwitch { get; set; } | ||
| public AutoModeSwitchHandler? OnAutoModeSwitchRequest { get; set; } |
There was a problem hiding this comment.
Cross-SDK consistency note: Same naming divergence as OnExitPlanModeRequest — the other SDKs use onAutoModeSwitch / on_auto_mode_switch / OnAutoModeSwitch (without the Request suffix). These two renames should be treated together if they're carried through to the other SDKs.
| [JsonConverter(typeof(JsonStringEnumConverter<ConnectionState>))] | ||
| public enum ConnectionState | ||
| [DebuggerDisplay("{Value,nq}")] | ||
| public readonly struct CopilotLogLevel : IEquatable<CopilotLogLevel> |
There was a problem hiding this comment.
This is fine if you think it's useful to maintain a separate type, but we're already using ILogger{Factory}... should we just use LogLevel here? Internally we can translate it to whatever the runtime needs.
There was a problem hiding this comment.
I started with LogLevel but started thinking nothing stops the runtime from coming up with new loglevel values at any time that could have any semantics, without being constrained to things that match a .NET LogLevel. My guess is this won't inconvenience .NET devs too much since they aren't programmatically mapping some other loglevel into this in most cases, so they will still just pick whatever level they think is right. We could add an implicit conversion from LogLevel later if we want.
Unmappable / arbitrary-JSON schema nodes now emit JsonElement instead of object in both Generated/SessionEvents.cs and Generated/Rpc.cs. Internal hand-written types that round-trip arbitrary JSON (ToolInvocation.Arguments, ToolResultObject.ToolTelemetry, hook *Input* ToolArgs/ToolResult) are also retyped to JsonElement. Hook *Output* fields, ElicitationSchema.Properties, ElicitationResult.Content, and PermissionRequestResult.Rules remain object- typed to keep consumer-side construction ergonomic; conversion happens at the wire boundary inside the SDK. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Seal all hand-written leaf classes in Types.cs (70 classes). Kept non-sealed: - McpServerConfig: existing polymorphic abstract base - SessionConfig / ResumeSessionConfig: will be refactored to share a base in Phase 5 - SessionLifecycleEvent: will become polymorphic base in Phase 4 - CopilotClientOptions / MessageOptions: existing protected Clone(...) ctor + virtual Clone() pattern - SessionFsProvider: existing abstract base intended for consumer subclassing Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- CopilotSession.GetMessagesAsync -> GetEventsAsync - (Resume)SessionConfig.OnExitPlanMode -> OnExitPlanModeRequest - (Resume)SessionConfig.OnAutoModeSwitch -> OnAutoModeSwitchRequest - (Resume)SessionConfig.CreateSessionFsHandler -> CreateSessionFsProvider - ResumeSessionConfig.DisableResume -> SuppressResumeEvent - ProviderConfig.MaxInputTokens -> MaxPromptTokens - InputOptions -> UiInputOptions - ISessionUiApi.ElicitationAsync -> ElicitAsync Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Per ApiView #278: - RmAsync -> RemoveAsync - MkdirAsync -> MakeDirectoryAsync - ReaddirAsync -> ReadDirectoryAsync - ReaddirWithTypesAsync -> ReadDirectoryWithTypesAsync Applied to the consumer-facing protected abstract methods on SessionFsProvider. The generated ISessionFsHandler interface keeps the original RPC-style names; SessionFsProvider's explicit interface impls bridge to the new spelled-out method names. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nups)
- (Resume)SessionConfig.Streaming: bool -> bool? (JsonIgnore WhenWritingNull)
- McpServerConfig.Tools: IList<string> -> IList<string>? (null = include all,
empty = include none); auto-init removed
- ToolBinaryResult.Type: bare string -> string-backed struct
ToolBinaryResultType with well-known Image / Resource values
- Remove obsolete redirects:
- CopilotClientOptions.GithubToken / .AutoRestart
- PermissionRequestResultKind.DeniedInteractivelyByUser /
DeniedCouldNotRequestFromUser / DeniedByRules
- Remove CopilotClientOptions.AutoStart (auto-start always on; consumer can
still call StartAsync explicitly)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Remove CopilotClient.State and the ConnectionState enum (per ApiView decision: 'remove this property') - Remove the _disconnected field that backed State - Convert CopilotClientOptions.LogLevel from bare string with default 'info' to nullable CopilotLogLevel (string-backed struct in the same SDK house style as PermissionRequestResultKind/ToolBinaryResultType) with well-known values: None/Error/Warning/Info/Debug/All (matching the runtime's accepted LOG_LEVELS array). When null, the --log-level arg is omitted, which is equivalent to the runtime's 'default' value. - Don't expose the values as an enum — the runtime doesn't necessarily treat them as a linear scale, so '<'/'>' comparisons would be misleading. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A PowerShell editing pipeline in earlier Phase 4 commits read UTF-8 source bytes through a cp1252 decode, corrupting em-dashes (U+2014, UTF-8 E2 80 94) into 'â€\u201d' sequences (UTF-8 C3 A2 E2 82 AC E2 80 9D). Inverse-encoded the affected files via a small fix-mojibake.ps1 utility (in session state) that handles UTF-8 BOMs correctly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add SessionCreatedEvent / SessionDeletedEvent / SessionUpdatedEvent / SessionForegroundEvent / SessionBackgroundEvent as sealed derived types of SessionLifecycleEvent. Unknown future event types deserialize to the base SessionLifecycleEvent for forward compatibility (consumers can still inspect Type). - Remove the SessionLifecycleEventTypes static constants class (subsumed by the typed hierarchy). - Replace CopilotClient.On / CopilotClient.On(string, ...) with a single generic OnLifecycle<T>(Action<T>) where T : SessionLifecycleEvent. Consumers pass a derived type to filter, or SessionLifecycleEvent to receive all. - Replace CopilotSession.On(SessionEventHandler) with generic On<T>(Action<T>) where T : SessionEvent. Same filtering pattern. - Remove the SessionEventHandler delegate; SessionConfig.OnEvent / ResumeSessionConfig.OnEvent are now Action<SessionEvent>?. - Update samples, tests, and README. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add UnixMillisecondsDateTimeOffsetConverter for unix-millis-as-JSON-number timestamps - Convert long Timestamp fields on hook *Input* types and PingResponse to DateTimeOffset via the new converter - Convert SessionLifecycleEventMetadata.StartTime/ModifiedTime from ISO 8601 strings to DateTimeOffset - Convert SessionMetadata.StartTime/ModifiedTime from DateTime to DateTimeOffset - Add PermissionRequestResult.Feedback property (parity with the RPC PermissionDecisionReject type's feedback field); forward to the wire only on reject decisions - Tidy XML doc on PermissionRequestResult.Kind to reference the well-known PermissionRequestResultKind static members directly Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…rloads
Most SDK uses end up looking like SendAsync(new MessageOptions { Prompt = ... });
add a plain-prompt overload that wraps it. Per ApiView #258.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- CopilotClientOptions.Cwd -> WorkingDirectory - CopilotClientOptions.Remote -> EnableRemoteSessions - McpStdioServerConfig.Cwd -> WorkingDirectory (wire name stays 'cwd') - SessionListFilter.Cwd -> WorkingDirectory Full CLI/server -> runtime terminology overhaul (the RuntimeConnection discriminated config and removal of flat CliPath/CliArgs/Port/UseStdio/CliUrl properties) is deferred — it requires substantial restructuring of the connection-routing logic in Client.cs and warrants its own change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace stale references to SessionLifecycleEventTypes constants with the new typed hierarchy (SessionCreatedEvent / SessionDeletedEvent / etc.). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SteveSandersonMS
force-pushed
the
stevesandersonms/stevesa-api-review-fixes
branch
from
9f16314 to
a64803a
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1343 · ● 1.3M
| /// </code> | ||
| /// </example> | ||
| public async Task<IReadOnlyList<SessionEvent>> GetMessagesAsync(CancellationToken cancellationToken = default) | ||
| public async Task<IReadOnlyList<SessionEvent>> GetEventsAsync(CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
Cross-SDK inconsistency: GetEventsAsync vs getMessages
All other SDK implementations use the "Messages" naming for this method:
- Node.js:
session.getMessages()(nodejs/src/session.ts) - Python:
session.get_messages()(python/copilot/session.py) - Go:
session.GetMessages(ctx)(go/session.go) - Rust:
GetMessagesResponsestruct (rust/src/types.rs)
The .NET rename to GetEventsAsync diverges from the cross-SDK convention. Consider whether the other SDKs should also be updated to use "Events" naming, or whether this should remain GetMessagesAsync for consistency.
| /// When provided, the server will route <c>exitPlanMode.request</c> callbacks to this handler. | ||
| /// </summary> | ||
| public ExitPlanModeHandler? OnExitPlanMode { get; set; } | ||
| public ExitPlanModeHandler? OnExitPlanModeRequest { get; set; } |
There was a problem hiding this comment.
Cross-SDK inconsistency: OnExitPlanModeRequest vs onExitPlanMode
Other SDKs use onExitPlanMode (without the Request suffix):
- Node.js:
onExitPlanMode?: ExitPlanModeHandler(nodejs/src/types.ts) - Python:
on_exit_plan_mode: ExitPlanModeHandler(python/copilot/session.py) - Go:
OnExitPlanMode ExitPlanModeHandler(go/types.go) - Rust:
on_exit_plan_modemethod (rust/src/handler.rs)
The added Request suffix here (and for OnAutoModeSwitchRequest below) makes the semantics more explicit, but creates a naming divergence from all other language SDKs. If the "Request" suffix is the preferred direction, the other SDKs should be updated to match.
| /// Default: false (resume event is emitted). | ||
| /// </summary> | ||
| public bool DisableResume { get; set; } | ||
| public bool SuppressResumeEvent { get; set; } |
There was a problem hiding this comment.
Cross-SDK inconsistency: SuppressResumeEvent vs disableResume
Other SDKs all use disableResume / disable_resume:
- Node.js:
disableResume?: boolean(nodejs/src/types.ts) - Python:
disable_resume: bool(python/copilot/session.py) - Go:
DisableResume bool(go/types.go) - Rust:
disable_resume: Option<bool>(rust/src/types.rs)
SuppressResumeEvent is semantically more descriptive, but it's a significant divergence from the cross-SDK disableResume convention. This is worth aligning across all SDKs if the new name is preferred.
| /// </summary> | ||
| [JsonPropertyName("maxPromptTokens")] | ||
| public int? MaxInputTokens { get; set; } | ||
| public int? MaxPromptTokens { get; set; } |
There was a problem hiding this comment.
Cross-SDK inconsistency: MaxPromptTokens vs maxInputTokens
Other SDKs expose this as maxInputTokens / MaxInputTokens in their public APIs (mapping to maxPromptTokens on the wire):
- Node.js:
maxInputTokens?: numberinProviderConfig(nodejs/src/types.ts:1676), mapped tomaxPromptTokenswire format inclient.ts - Go:
MaxInputTokens intinProviderConfig(go/types.go:991), tagged withjson:"maxPromptTokens,omitempty"
The .NET SDK now exposes MaxPromptTokens directly, which aligns with the wire format but diverges from the other SDKs' public APIs. If MaxPromptTokens is the better name, consider updating Node.js and Go as well.
- New abstract SessionConfigBase holds the 32 properties duplicated between SessionConfig and ResumeSessionConfig, plus the shared copy-constructor logic. - SessionConfig now contains only SessionId + Cloud. - ResumeSessionConfig now contains only SuppressResumeEvent + ContinuePendingWork. - SessionConfig, ResumeSessionConfig, CopilotClientOptions, and MessageOptions are now sealed. Each has a private copy constructor + non-virtual Clone() method (was protected/virtual to allow subclassing — no longer a goal). - Updated <see cref> references from SessionConfig.X to SessionConfigBase.X where the property lives on the base. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Drop these 13 named delegate types in favor of inline Func<...>: - ToolHandler (unused; just deleted) - PermissionRequestHandler -> Func<PermissionRequest, PermissionInvocation, Task<PermissionRequestResult>> - UserInputHandler - ExitPlanModeHandler - AutoModeSwitchHandler - CommandHandler -> Func<CommandContext, Task> - ElicitationHandler -> Func<ElicitationContext, Task<ElicitationResult>> - PreToolUseHandler / PostToolUseHandler / UserPromptSubmittedHandler - SessionStartHandler / SessionEndHandler / ErrorOccurredHandler Lambdas at call sites are unaffected (target-typed). The PermissionHandler static class keeps its ApproveAll preset; the property type is now Func<...>. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Replaces the flat CliPath/CliArgs/Port/UseStdio/CliUrl/TcpConnectionToken properties on CopilotClientOptions with a single Connection property of type RuntimeConnection, constructed via factory methods: - RuntimeConnection.Stdio(path?, args?) - spawns runtime over stdio (default) - RuntimeConnection.Tcp(port=0, connectionToken?, path?, args?) - spawns runtime listening on TCP. port=0 auto-allocates; port-in-use is an error. - RuntimeConnection.Uri(url, connectionToken?) - connects to an already running runtime; does not spawn. The 'exactly one mode' invariant is now encoded by the type hierarchy (ChildProcessRuntimeConnection abstract base for the spawning kinds, plus UriRuntimeConnection for the connect-to-existing case). Connection token only exists on Tcp/Uri (Stdio doesn't need one). Concrete subclasses have internal ctors so factory methods are the only way to construct. Client.cs stores the RuntimeConnection directly and pattern-matches on the subtype at the connection-routing points (no separate flat fields). Also: - Rename CopilotClient.ActualPort -> RuntimePort. - Rename CopilotClientOptions.CopilotHome -> BaseDirectory. - README updated to describe the new Connection model and removed flags. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… removal Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When a caller passes both, intent is ambiguous and easy to get wrong (this was the source of the recent extension-test regression where the useStdio parameter was silently ignored). Throw eagerly so the mistake shows up immediately, and drop the now-redundant useStdio:false arg from every call site that also sets Connection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1343 · ● 1.1M
| /// </summary> | ||
| public ExitPlanModeHandler? OnExitPlanMode { get; set; } | ||
| /// <summary>Handler for exit-plan-mode requests from the server.</summary> | ||
| public Func<ExitPlanModeRequest, ExitPlanModeInvocation, Task<ExitPlanModeResult>>? OnExitPlanModeRequest { get; set; } |
There was a problem hiding this comment.
Cross-SDK consistency note: The .NET rename from OnExitPlanMode → OnExitPlanModeRequest (and OnAutoModeSwitch → OnAutoModeSwitchRequest on the next line) diverges from all other SDKs:
- Node.js:
onExitPlanMode/onAutoModeSwitch - Python:
on_exit_plan_mode/on_auto_mode_switch - Go:
OnExitPlanMode/OnAutoModeSwitch
If the *Request suffix is the correct semantic name (these are requests that the server sends to the client), it would be worth applying the same rename in the other SDKs too. If it's a .NET-only style choice, that's fine but worth documenting the intentional divergence.
| /// This is used only when <see cref="CopilotClientOptions.SessionFs"/> is configured. | ||
| /// </summary> | ||
| public Func<CopilotSession, SessionFsProvider>? CreateSessionFsHandler { get; set; } | ||
| public Func<CopilotSession, SessionFsProvider>? CreateSessionFsProvider { get; set; } |
There was a problem hiding this comment.
Cross-SDK consistency note: CreateSessionFsProvider diverges from the name used in all other SDKs:
- Node.js:
createSessionFsHandler - Python:
create_session_fs_handler - Go:
CreateSessionFsHandler
If this rename from Handler → Provider is intentional (the field holds a factory for a SessionFsProvider, so "Provider" is semantically clearer), the same rename could be applied to the other SDKs for consistency.
| /// Gets the actual TCP port the runtime is listening on, if using TCP transport. | ||
| /// </summary> | ||
| public int? ActualPort => _actualPort; | ||
| public int? RuntimePort => _actualPort; |
There was a problem hiding this comment.
Cross-SDK consistency note: RuntimePort diverges from the name used in other SDKs:
- Node.js:
actualPort(private field — not exposed as a public property) - Python:
actual_port(public property) - Go:
ActualPort()(public method)
If RuntimePort is the preferred name going forward, the Python and Go SDKs could be updated. Node.js could also expose this publicly.
| /// </code> | ||
| /// </example> | ||
| public IDisposable On(SessionEventHandler handler) | ||
| public IDisposable On<T>(Action<T> handler) where T : SessionEvent |
There was a problem hiding this comment.
Cross-SDK opportunity: The new On<T>() generic method (where T : SessionEvent) allows callers to filter event subscriptions to a specific event type at compile time — a nice improvement. This capability doesn't currently exist in other SDKs:
- Node.js:
session.on(handler)— all events, no type filtering - Python:
session.on(handler)— all events, no type filtering - Go:
session.On(handler)— all events, no type filtering
Worth considering adding equivalent typed filtering to the other SDKs in a follow-up.
| /// when connecting to an external runtime via <see cref="RuntimeConnection.Uri(string, string?)"/>. | ||
| /// </summary> | ||
| public bool Remote { get; set; } | ||
| public bool EnableRemoteSessions { get; set; } |
There was a problem hiding this comment.
Cross-SDK consistency note: EnableRemoteSessions diverges from the name used in all other SDKs:
- Node.js:
remote - Python:
remote - Go:
Remote
Similarly, BaseDirectory (line ~260) diverges from CopilotHome / copilotHome / copilot_home in the other SDKs, and WorkingDirectory at the client options level (line ~251) diverges from Cwd / cwd in Go, Node.js, and Python.
If these renames are considered improvements (e.g., EnableRemoteSessions is more self-documenting than Remote), it would be worth aligning the other SDKs in follow-up PRs.
| /// <param name="prompt">The user message text.</param> | ||
| /// <param name="cancellationToken">A <see cref="CancellationToken"/> that can be used to cancel the operation.</param> | ||
| /// <returns>A task that resolves with the message ID.</returns> | ||
| public Task<string> SendAsync(string prompt, CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
Cross-SDK opportunity: The new SendAsync(string prompt) and SendAndWaitAsync(string prompt) string overloads are a convenience improvement. Status across other SDKs:
- Python: Already accepts
prompt: strdirectly — consistent ✅ - Go:
Send(ctx, MessageOptions)requires a struct — no string shorthand - Node.js:
send({ prompt: "..." })requires an object — no string shorthand
Go and Node.js could benefit from adding equivalent string convenience overloads/variants.
Replace CliPath/CliArgs/CliUrl/UseStdio with RuntimeConnection.Stdio/Tcp/Uri, CopilotHome with BaseDirectory, McpStdioServerConfig.Cwd with WorkingDirectory, GithubToken with GitHubToken, and add explicit type args to session.On() / client.OnLifecycle() calls in C# code blocks across the docs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace CliPath/CliUrl with RuntimeConnection.Stdio/Uri and add explicit type args to session.On() across all C# scenario programs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1343 · ● 1.9M
| /// Default: false (resume event is emitted). | ||
| /// </summary> | ||
| public bool DisableResume { get; set; } | ||
| public bool SuppressResumeEvent { get; set; } |
There was a problem hiding this comment.
Cross-SDK consistency note: SuppressResumeEvent diverges from the other SDK implementations, which all use DisableResume / disableResume / disable_resume:
- Go:
DisableResume bool(go/types.go:932) - Python:
disable_resume: bool(python/copilot/session.py:1062) - Node.js:
disableResume?: boolean(nodejs/src/types.ts:1593)
Worth aligning on a single canonical name if this is an intentional rename, or tracking a follow-up to update the other SDKs.
| /// </code> | ||
| /// </example> | ||
| public async Task<IReadOnlyList<SessionEvent>> GetMessagesAsync(CancellationToken cancellationToken = default) | ||
| public async Task<IReadOnlyList<SessionEvent>> GetEventsAsync(CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
Cross-SDK consistency note: GetEventsAsync diverges from the other SDK implementations, which all use GetMessages / get_messages:
- Go:
func (s *Session) GetMessages(ctx context.Context)(go/session.go:1170) - Python:
async def get_messages(self)(python/copilot/session.py:2217) - Node.js:
async getMessages()(nodejs/src/session.ts:994)
If GetEvents is the preferred canonical name going forward, the other three SDKs would need follow-up updates.
| /// </summary> | ||
| [JsonPropertyName("maxPromptTokens")] | ||
| public int? MaxInputTokens { get; set; } | ||
| public int? MaxPromptTokens { get; set; } |
There was a problem hiding this comment.
Cross-SDK consistency note: MaxPromptTokens diverges from the other SDKs' public ProviderConfig API, which all expose this as MaxInputTokens / maxInputTokens / max_input_tokens (each transparently remapping to the wire field maxPromptTokens):
- Go:
MaxInputTokens int→json:"maxPromptTokens"(go/types.go:991) - Python:
max_input_tokens: int(mapped to wiremaxPromptTokensinclient.py:2533) - Node.js:
maxInputTokens?: number(mapped to wiremaxPromptTokensinclient.ts:78)
Renaming only the .NET property creates an inconsistency. If MaxPromptTokens is the preferred name, the other SDKs should follow.
| /// Gets the actual TCP port the runtime is listening on, if using TCP transport. | ||
| /// </summary> | ||
| public int? ActualPort => _actualPort; | ||
| public int? RuntimePort => _actualPort; |
There was a problem hiding this comment.
Cross-SDK consistency note: RuntimePort diverges from the other SDKs, which use ActualPort / actual_port:
- Go:
func (c *Client) ActualPort() int(go/client.go:1300) - Python:
def actual_port(self) -> int | None(python/copilot/client.py:1026)
If RuntimePort is the preferred name, consider updating the Go and Python equivalents in a follow-up.
| /// This is used only when <see cref="CopilotClientOptions.SessionFs"/> is configured. | ||
| /// </summary> | ||
| public Func<CopilotSession, SessionFsProvider>? CreateSessionFsHandler { get; set; } | ||
| public Func<CopilotSession, SessionFsProvider>? CreateSessionFsProvider { get; set; } |
There was a problem hiding this comment.
Cross-SDK consistency note: CreateSessionFsProvider diverges from the other SDKs, which use createSessionFsHandler / create_session_fs_handler:
- Node.js:
createSessionFsHandler?inSessionConfig(nodejs/src/types.ts:1545) - Python:
create_session_fs_handler: CreateSessionFsHandler(python/copilot/session.py:994)
This is a minor naming difference, but worth tracking for eventual alignment.
GitHub.Copilot.SDK -> GitHub.Copilot (and all sub-namespaces in step). The package name and csproj/.targets/.props filenames still include .SDK, so RootNamespace is set explicitly on both csproj files to override the default derived from the assembly name. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR applies a large set of API-review-driven breaking changes to the .NET Copilot SDK, updating the public surface area (connection configuration, typed events, naming/casing, value types) and migrating tests, samples, and docs accordingly.
Changes:
- Replaces flat transport options (
CliPath/CliUrl/UseStdio/Port/TcpConnectionToken) with a discriminatedRuntimeConnection(Stdio/Tcp/Uri) onCopilotClientOptions. - Introduces stronger typing and API reshaping (e.g.,
CopilotLogLevel, typedOnLifecycle<T>/session.On<T>,GetEventsAsync,ToolBinaryResultType,DateTimeOffsettimestamps). - Updates all .NET tests, scenario programs, samples, and documentation to the new API shape.
Show a summary per file
| File | Description |
|---|---|
| test/scenarios/transport/tcp/csharp/Program.cs | Updates scenario to use RuntimeConnection.Uri(...). |
| test/scenarios/transport/stdio/csharp/Program.cs | Updates scenario to use RuntimeConnection.Stdio(...). |
| test/scenarios/transport/reconnect/csharp/Program.cs | Updates reconnect scenario to RuntimeConnection.Uri(...). |
| test/scenarios/tools/virtual-filesystem/csharp/Program.cs | Migrates to RuntimeConnection.Stdio(...). |
| test/scenarios/tools/tool-overrides/csharp/Program.cs | Migrates to RuntimeConnection.Stdio(...). |
| test/scenarios/tools/tool-filtering/csharp/Program.cs | Migrates to RuntimeConnection.Stdio(...). |
| test/scenarios/tools/skills/csharp/Program.cs | Migrates to RuntimeConnection.Stdio(...). |
| test/scenarios/tools/no-tools/csharp/Program.cs | Migrates to RuntimeConnection.Stdio(...). |
| test/scenarios/tools/mcp-servers/csharp/Program.cs | Migrates to RuntimeConnection.Stdio(...). |
| test/scenarios/tools/custom-agents/csharp/Program.cs | Migrates to RuntimeConnection.Stdio(...). |
| test/scenarios/sessions/streaming/csharp/Program.cs | Updates connection config and typed event subscription (On<SessionEvent>). |
| test/scenarios/sessions/session-resume/csharp/Program.cs | Migrates to RuntimeConnection.Stdio(...). |
| test/scenarios/sessions/infinite-sessions/csharp/Program.cs | Migrates to RuntimeConnection.Stdio(...). |
| test/scenarios/sessions/concurrent-sessions/csharp/Program.cs | Migrates to RuntimeConnection.Stdio(...). |
| test/scenarios/prompts/system-message/csharp/Program.cs | Migrates to RuntimeConnection.Stdio(...). |
| test/scenarios/prompts/reasoning-effort/csharp/Program.cs | Migrates to RuntimeConnection.Stdio(...). |
| test/scenarios/prompts/attachments/csharp/Program.cs | Migrates to RuntimeConnection.Stdio(...). |
| test/scenarios/modes/minimal/csharp/Program.cs | Migrates to RuntimeConnection.Stdio(...). |
| test/scenarios/modes/default/csharp/Program.cs | Migrates to RuntimeConnection.Stdio(...). |
| test/scenarios/callbacks/user-input/csharp/Program.cs | Migrates to RuntimeConnection.Stdio(...). |
| test/scenarios/callbacks/permissions/csharp/Program.cs | Migrates to RuntimeConnection.Stdio(...). |
| test/scenarios/callbacks/hooks/csharp/Program.cs | Migrates to RuntimeConnection.Stdio(...). |
| test/scenarios/bundling/fully-bundled/csharp/Program.cs | Migrates to RuntimeConnection.Stdio(...). |
| test/scenarios/bundling/container-proxy/csharp/Program.cs | Migrates to RuntimeConnection.Uri(...). |
| test/scenarios/bundling/app-direct-server/csharp/Program.cs | Migrates to RuntimeConnection.Uri(...). |
| test/scenarios/bundling/app-backend-to-server/csharp/Program.cs | Migrates to RuntimeConnection.Uri(...). |
| test/scenarios/auth/gh-app/csharp/Program.cs | Migrates to RuntimeConnection.Stdio(...). |
| test/scenarios/auth/byok-openai/csharp/Program.cs | Migrates to RuntimeConnection.Stdio(...). |
| test/scenarios/auth/byok-ollama/csharp/Program.cs | Migrates to RuntimeConnection.Stdio(...). |
| test/scenarios/auth/byok-azure/csharp/Program.cs | Migrates to RuntimeConnection.Stdio(...). |
| test/scenarios/auth/byok-anthropic/csharp/Program.cs | Migrates to RuntimeConnection.Stdio(...). |
| dotnet/test/Unit/SerializationTests.cs | Updates serialization expectations for renamed/reshaped properties. |
| dotnet/test/Unit/PermissionRequestResultKindTests.cs | Removes assertions for deprecated kind aliases. |
| dotnet/test/Unit/CloneTests.cs | Updates cloning tests for Connection, renamed options, and renamed handlers. |
| dotnet/test/Unit/ClientSessionLifetimeTests.cs | Updates client creation to use RuntimeConnection.Uri(...). |
| dotnet/test/Harness/TestHelper.cs | Migrates helper logic to On<SessionEvent> and GetEventsAsync(). |
| dotnet/test/Harness/E2ETestFixture.cs | Updates shared client construction to RuntimeConnection.Tcp(...). |
| dotnet/test/Harness/E2ETestContext.cs | Reworks harness client creation around Connection and tightens auth/token injection behavior. |
| dotnet/test/Harness/E2ETestBase.cs | Updates multi-client resume flow to RuntimePort + RuntimeConnection.Uri(...). |
| dotnet/test/E2E/ToolsE2ETests.cs | Updates binary tool result typing (ToolBinaryResultType). |
| dotnet/test/E2E/ToolResultsE2ETests.cs | Migrates event subscriptions to typed On<SessionEvent>. |
| dotnet/test/E2E/SuspendE2ETests.cs | Updates TCP/URI connection creation and RuntimePort usage. |
| dotnet/test/E2E/StreamingFidelityE2ETests.cs | Migrates to On<SessionEvent> and GetEventsAsync(). |
| dotnet/test/E2E/SessionMcpAndAgentConfigE2ETests.cs | Renames MCP working directory property and updates config usage. |
| dotnet/test/E2E/SessionLifecycleE2ETests.cs | Migrates persisted-history API to GetEventsAsync(). |
| dotnet/test/E2E/SessionFsSqliteE2ETests.cs | Renames session-fs factory to CreateSessionFsProvider and typed On<SessionEvent>. |
| dotnet/test/E2E/SessionFsE2ETests.cs | Updates session-fs APIs, connection options, and event/history APIs. |
| dotnet/test/E2E/SessionE2ETests.cs | Updates event APIs, typed subscriptions, and history retrieval naming. |
| dotnet/test/E2E/SessionConfigE2ETests.cs | Updates event history API name and related assertions. |
| dotnet/test/E2E/RpcShellAndFleetE2ETests.cs | Migrates polling helper to GetEventsAsync(). |
| dotnet/test/E2E/RpcSessionStateE2ETests.cs | Migrates persisted history retrieval to GetEventsAsync(). |
| dotnet/test/E2E/RpcMcpAndSkillsE2ETests.cs | Moves CliArgs usage into RuntimeConnection.Stdio(args: ...). |
| dotnet/test/E2E/RpcExtensionsLoadedE2ETests.cs | Moves CliArgs usage into RuntimeConnection.Stdio(args: ...). |
| dotnet/test/E2E/RpcEventSideEffectsE2ETests.cs | Migrates persisted history retrieval to GetEventsAsync(). |
| dotnet/test/E2E/PermissionE2ETests.cs | Migrates all subscriptions to typed On<SessionEvent>. |
| dotnet/test/E2E/PendingWorkResumeE2ETests.cs | Updates TCP/URI client creation and typed subscriptions for pending-work resume. |
| dotnet/test/E2E/MultiTurnE2ETests.cs | Migrates subscriptions to typed On<SessionEvent>. |
| dotnet/test/E2E/MultiClientE2ETests.cs | Updates shared TCP server creation, URI client creation, and typed subscriptions. |
| dotnet/test/E2E/MultiClientCommandsElicitationE2ETests.cs | Updates TCP/URI setup, typed subscriptions, and SuppressResumeEvent rename. |
| dotnet/test/E2E/ModeHandlersE2ETests.cs | Renames mode-switch handlers and updates typed On<SessionEvent>. |
| dotnet/test/E2E/InMemorySessionFsSqliteHandler.cs | Updates abstract method override names in session-fs provider. |
| dotnet/test/E2E/HookLifecycleAndOutputE2ETests.cs | Updates hook timestamp assertions to DateTimeOffset. |
| dotnet/test/E2E/EventFidelityE2ETests.cs | Migrates subscriptions and event-history retrieval to new APIs. |
| dotnet/test/E2E/ErrorResilienceE2ETests.cs | Updates disposed-session history call to GetEventsAsync(). |
| dotnet/test/E2E/ElicitationE2ETests.cs | Renames UI method to ElicitAsync, input options to UiInputOptions, and removes named delegate usage. |
| dotnet/test/E2E/CompactionE2ETests.cs | Migrates subscription to typed On<SessionEvent>. |
| dotnet/test/E2E/ClientOptionsE2ETests.cs | Migrates to RuntimeConnection, RuntimePort, and renamed option properties/types. |
| dotnet/test/E2E/ClientLifecycleE2ETests.cs | Migrates lifecycle subscriptions to OnLifecycle<T> and typed lifecycle event classes. |
| dotnet/test/E2E/ClientE2ETests.cs | Migrates transport selection to RuntimeConnection factories. |
| dotnet/test/E2E/AbortE2ETests.cs | Migrates subscriptions to typed On<SessionEvent>. |
| dotnet/test/ConnectionTokenTests.cs | Migrates TCP/URI token plumbing to RuntimeConnection. |
| dotnet/src/UnixMillisecondsDateTimeOffsetConverter.cs | Adds a converter for unix-ms timestamps to DateTimeOffset. |
| dotnet/src/Types.cs | Major public API reshaping: connection config, log level value type, typed lifecycle events, renames, DateTimeOffset, tool binary type, config base extraction, sealing types. |
| dotnet/src/SessionFsProvider.cs | Renames abstract methods to expanded names and seals result type. |
| dotnet/src/Session.cs | Adds SendAsync(string)/SendAndWaitAsync(string) overloads, introduces On<T>, renames history to GetEventsAsync(), updates handler delegate types. |
| dotnet/src/PermissionHandlers.cs | Updates built-in handler type to Func<...>-based signature. |
| dotnet/src/CopilotTool.cs | Updates docs to reference SessionConfigBase. |
| dotnet/src/Client.cs | Implements RuntimeConnection, typed lifecycle subscriptions, updated wire request shapes, and removes State surface. |
| dotnet/samples/ManualToolResume.cs | Updates sample to typed On<SessionEvent>. |
| dotnet/samples/Chat.cs | Updates sample to typed On<SessionEvent>. |
| dotnet/README.md | Updates .NET SDK docs to new API surface (connection config, lifecycle APIs, event APIs, elicitation renames). |
| docs/troubleshooting/mcp-debugging.md | Updates MCP stdio server working directory property name. |
| docs/troubleshooting/debugging.md | Updates .NET example to use RuntimeConnection.Stdio(args: ...). |
| docs/setup/local-cli.md | Updates .NET setup snippet to use RuntimeConnection.Stdio(path: ...). |
| docs/setup/github-oauth.md | Corrects GithubToken casing to GitHubToken in docs. |
| docs/setup/backend-services.md | Updates .NET backend-services snippets to RuntimeConnection.Uri(...). |
| docs/getting-started.md | Updates .NET getting-started snippet to RuntimeConnection.Uri(...) and typed On<SessionEvent>. |
| docs/features/streaming-events.md | Migrates examples to typed On<SessionEvent>. |
| docs/features/custom-agents.md | Migrates examples to typed On<SessionEvent>. |
| docs/auth/authenticate.md | Corrects GithubToken casing to GitHubToken in docs. |
Copilot's findings
- Files reviewed: 138/140 changed files
- Comments generated: 6
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- README: fix Custom Permission Handler signature (PermissionInvocation, not ToolInvocation). - Client.cs StartAsync: remove empty <para/> in remarks and the misleading 'Reset disposed flag for restart scenarios' comment (no reset actually happens). - Indent the Connection = RuntimeConnection.Uri(...) line inside object initializers in backend-services.md and getting-started.md C# snippets for consistency. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The validation extractor auto-injects a using directive into every C# snippet; update it to match the new namespace. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| { | ||
| continue; | ||
| } | ||
| try { subscription.Handler(evt); } catch { /* Ignore handler errors */ } |
Cross-SDK Consistency ReviewThanks for the detailed PR description — the phased breakdown makes this easy to trace. Since this is explicitly a .NET-only API review pass with planned future ports, most of the changes are intentional divergences. The notes below are meant to serve as a reference for the "port what makes sense" follow-up work. ✅ Already consistent / no action needed
📌 Items to track for future cross-SDK workPhase 7 – These are the most directly portable ergonomic improvements:
Phase 9 – The .NET refactor moves from flat mutually-exclusive fields (
All three would benefit from the discriminated-union approach to make invalid config a compile-time (or at least early-init) error rather than a runtime surprise. That said, this is a breaking API change for those SDKs, so flagging for future consideration rather than blocking. No blocking issues — the PR is well-scoped and the description is transparent about the intentional divergence. The two items above (Phase 7 convenience overloads for Node.js/Go, Phase 9 connection config shape) are worth filing follow-up tracking issues against if they aren't already captured elsewhere.
|
| { | ||
| CliUrl = "localhost:4321", | ||
| UseStdio = false, | ||
| Connection = RuntimeConnection.Uri("localhost:4321"), |
There was a problem hiding this comment.
This is missing a verb or a preposition. How about RuntimeConnection.ForUri?
| var client = new CopilotClient(new CopilotClientOptions | ||
| { | ||
| CliPath = "/usr/local/bin/copilot", | ||
| Connection = RuntimeConnection.Stdio(path: "/usr/local/bin/copilot"), |
There was a problem hiding this comment.
Similarly here, ForStdio or CreateWithStdio or something like that?
|
|
||
| ```bash | ||
| dotnet add package GitHub.Copilot.SDK | ||
| dotnet add package GitHub.Copilot |
There was a problem hiding this comment.
Is the package name changing?
| public override ModelPickerCategory Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) | ||
| { | ||
| return new(GitHub.Copilot.SDK.GeneratedStringEnumJson.ReadValue(ref reader, typeToConvert)); | ||
| return new(GitHub.Copilot.GeneratedStringEnumJson.ReadValue(ref reader, typeToConvert)); |
There was a problem hiding this comment.
We shouldn't need the full qualification here, e.g.
| return new(GitHub.Copilot.GeneratedStringEnumJson.ReadValue(ref reader, typeToConvert)); | |
| return new(GeneratedStringEnumJson.ReadValue(ref reader, typeToConvert)); |
| <TargetFrameworks>net8.0;net10.0;netstandard2.0</TargetFrameworks> | ||
| <RootNamespace>GitHub.Copilot</RootNamespace> | ||
| <GenerateDocumentationFile>true</GenerateDocumentationFile> | ||
| <Version>0.1.0</Version> |
There was a problem hiding this comment.
nit: we override this when publishing, but maybe we should tweak it to be clear that it's a local build, e.g. 0.0.0-dev or something like that
| [JsonPropertyName("timestamp")] | ||
| public long Timestamp { get; set; } | ||
| [JsonConverter(typeof(UnixMillisecondsDateTimeOffsetConverter))] | ||
| public DateTimeOffset Timestamp { get; set; } |
There was a problem hiding this comment.
We're inconsistent right now in whether the runtime sends down timestamps as milliseconds since epoch or as an iso datetime string. We should ideally consolidate on one or the other.
Summary
API review fixes for the .NET SDK only. Sources combined into this PR:
api_review_csharp.mdreview doc.Other SDKs are not touched here; once we are happy with the .NET shape we will port what makes sense.
What changed (by phase)
SessionFsProvider.Streamingflag, tools list shape,ToolBinaryResultTypecleanup, misc.CopilotClient.Stateenum; retypedLogLevelto a strongly-typedCopilotLogLevelvalue type.client.OnLifecycle<T>/session.On<T>; lifecycle events are now a typed polymorphic hierarchy (SessionCreatedEvent, etc.) instead of stringly-typed discriminators.DateTimeOffsettimestamps;PermissionRequestResult.Feedbackcleanup.SessionConfigBasebetweenSessionConfig/ResumeSessionConfig; sealed all config classes.Func<...>.SendAsync(string)/SendAndWaitAsync(string)convenience overloads.RuntimeConnectiondiscriminated config: replaces the flatCliPath/CliArgs/Port/UseStdio/CliUrl/TcpConnectionTokenproperties onCopilotClientOptionswith a singleConnectionof typeRuntimeConnection, constructed via factories:RuntimeConnection.Stdio(...),RuntimeConnection.Tcp(...),RuntimeConnection.Uri(...). Also renamesActualPort->RuntimePortandCopilotHome->BaseDirectory.The test harness also got a small UX improvement:
Ctx.CreateClient(useStdio: ..., options: ... { Connection = ... })now throws eagerly if both are supplied. This caught a real bug introduced by Phase 9 bulk rewrite.Notable thing we explicitly did NOT do: Phase 1
Phase 1 was originally going to retype every "opaque JSON" RPC field from
objecttoSystem.Text.Json.JsonElement, on the theory thatJsonElementis more honest about wire shape thanobject(which cannot be safely cast). The change was reverted on this branch (see commits129c281eand074ace71) for two reasons:The underlying problem is in the runtime schema, not the SDK. Several RPC parameters/results are polymorphic
anyOfunions that have no idiomatic mapping in any of our SDK languages. Tracked in copilot-agent-runtime#8375: the schema generator should reject shapes that have no idiomatic mapping in C#/Go/Python/Rust, and require an explicit opt-in for cases that genuinely are meant to be opaque JSON. Once that lands, codegen will produce real typed hierarchies and theJsonElementworkaround stops being needed.Phase 1 asymmetry created a worse consumer experience for inputs. Changing input parameters from
objecttoJsonElementforced consumers to serialize themselves via their ownJsonSerializerOptions, which silently emitted"headers": nulland similar (becauseWhenWritingNullis not a default) and caused the runtime to reject otherwise-valid configs. On the previousobjectpath, the SDK ownJsonSerializerContext(which setsWhenWritingNull) was used, so this never bit consumers. We could have applied per-property[JsonIgnore(WhenWritingNull)]workarounds, but they are band-aids; the right fix is the schema change above. Reverting now keeps the API symmetric and aligned with main.We will revisit
JsonElementonce #8375 lands and codegen can emit real types.Risk / compatibility
This is a breaking-change PR by design. All E2E and unit tests have been updated in lock-step. The
dotnet/samplesanddocs/C# snippets have also been migrated.Two issues filed against the runtime as follow-ups to this work: